BytesIO.getbuffer() owner, a WTF-8 method cache, and three codegen/allocation cleanups - #1090
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
WalkthroughThe pull request updates JIT frame layouts and root synchronization, descriptor sharing and lookup, GC allocation lowering, memoryview ownership, builtin type creation, and LLBC freshness validation. It also adds regression and parity tests for these behaviors. ChangesPyFrame virtualizable layout
Compiler and lowering changes
Allocation and GC lowering
Interpreter runtime behavior
Bridge and execution support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cbe4088 to
9459884
Compare
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit e4d19ef). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
9459884 to
8301fa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/baseobjspace.rs (1)
9963-10013: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse own-dict lookup for the super binding target.
lookup_in_type_wtf8_uncached(t, name)also followst’s MRO. In an MRO likeD, B, C, A,B.fis not inB’s dict, so the outer loop can returnA.feven thoughC.fappears later inD’s own C3 MRO and wins insuper_getattribute_wtf8. Callcrate::type_dict_lookup_wtf8(t, name)so this function selects the same class thatw_obj_type’s MRO selected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/baseobjspace.rs` around lines 9963 - 10013, Update super_lookup_binding to use crate::type_dict_lookup_wtf8(t, name) instead of lookup_in_type_wtf8_uncached when inspecting each MRO class, so binding decisions use that class’s own dictionary and match super_getattribute_wtf8’s C3 MRO resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-translate/src/codewriter/assembler.rs`:
- Around line 3370-3374: Extract the generic-argument-stripping owner
normalization currently used by bh_size_spec_from_callcontrol into shared logic,
then apply it before both the parent field lookup and the struct_layout_for
fallback in inline_substruct_field_offset. Use the normalized owner for registry
lookups while preserving the existing BhFieldLookup mapping and Missing
behavior.
In `@majit/majit-translate/src/codewriter/jtransform.rs`:
- Around line 271-273: Update the FieldRead validation to account for transitive
call-argument uses rather than only the flat call_argument_vars list. Resolve
same_as aliases and values propagated through LinkArg into successor blocks
before deciding whether a FieldRead is a supported call argument, or move this
validation into optimize_block after operand remapping. Ensure every FieldRead
whose canonical value reaches any call argument is handled correctly.
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 9038-9069: Update lookup_where_pair_wtf8 so both valid and invalid
UTF-8 names route through lookup_where_pair_wtf8_uncached, replacing the
valid-UTF-8 call to lookup_where_pair. Preserve the existing Option pair return
behavior and ensure _cached_lookup_where_name uses the single-pass fallback for
all names.
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 367-392: Mark w_memoryview_new_simple_with_owner as unsafe and add
a # Safety documentation comment stating that w_backing must be a bytearray
before calling w_bytearray_exports_incref or w_bytearray_len. Update its callers
to use an unsafe block as required, and rename the w_obj parameter to w_owner if
consistent with the surrounding constructors and update its use in
BufferView::Simple.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 9963-10013: Update super_lookup_binding to use
crate::type_dict_lookup_wtf8(t, name) instead of lookup_in_type_wtf8_uncached
when inspecting each MRO class, so binding decisions use that class’s own
dictionary and match super_getattribute_wtf8’s C3 MRO resolution.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c96ccc6-a81b-4a2e-adb7-b3008fca327e
📒 Files selected for processing (36)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-gc/src/rewrite.rsmajit/majit-ir/src/descr.rsmajit/majit-macros/src/virtualizable/derive.rsmajit/majit-macros/src/virtualizable/mod.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/optimizeopt/info.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-translate/src/codewriter/assembler.rsmajit/majit-translate/src/codewriter/jtransform.rspyre/bench/synth/type_immutable_reject.pypyre/extra_tests/parity_tests/surrogate_method_cache_bases.pypyre/extra_tests/parity_tests/surrogate_method_cache_invalidation.pypyre/extra_tests/parity_tests/surrogate_method_cache_tag_zero.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/module/_io/bytesio.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/build.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/heapcache_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_runtime.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit-trace/src/virtualizable_gen.rspyre/pyre-jit-trace/src/virtualizable_spec.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/flatten.rs
💤 Files with no reviewable changes (2)
- pyre/pyre-jit-trace/src/descr.rs
- pyre/pyre-jit/src/call_jit.rs
| cc.struct_layout_for(owner) | ||
| .and_then(|layout| layout.fields.iter().find(|row| row.name == field.name)) | ||
| .cloned() | ||
| .map(BhFieldLookup::Layout) | ||
| .unwrap_or(BhFieldLookup::Missing) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize the owner before the layout fallback.
Line 3370 queries struct_layout_for with the raw owner. bh_size_spec_from_callcontrol strips generic arguments for this same registry lookup. For a by-value field on Outer<T>, the parent lookup can miss the unflattened struct field and this raw lookup can miss the registered Outer layout. inline_substruct_field_offset then returns None.
Extract the owner-normalization logic and use it in both lookup paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-translate/src/codewriter/assembler.rs` around lines 3370 - 3374,
Extract the generic-argument-stripping owner normalization currently used by
bh_size_spec_from_callcontrol into shared logic, then apply it before both the
parent field lookup and the struct_layout_for fallback in
inline_substruct_field_offset. Use the normalized owner for registry lookups
while preserving the existing BhFieldLookup mapping and Missing behavior.
| /// Results consumed as explicit arguments by a direct or indirect call in | ||
| /// the graph before call lowering rewrites those operations. | ||
| call_argument_vars: Vec<crate::flowspace::model::Variable>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'repo files likely relevant:'
git ls-files | rg '(^|/)majit-translate/src/codewriter/jtransform\.rs$|py$|rb$|rpy$' | sed -n '1,120p'
echo
echo 'target outline:'
ast-grep outline majit/majit-translate/src/codewriter/jtransform.rs --view expanded 2>/dev/null | sed -n '1,220p' || true
echo
echo 'target snippets:'
sed -n '240,295p' majit/majit-translate/src/codewriter/jtransform.rs
echo '--- 600-720 ---'
sed -n '600,720p' majit/majit-translate/src/codewriter/jtransform.rs
echo '--- 2500-2575 ---'
sed -n '2500,2575p' majit/majit-translate/src/codewriter/jtransform.rs
echo
echo 'search same_as/LinkArg/Abort/nonzero inline field read:'
rg -n "same_as|LinkArg|same.*as|FieldRead|inline_substructure|Abort|call_argument_vars|FieldSet|Inline|getsubstruct|new_substruct|getfield" majit/majit-translate/src/codewriter/jtransform.rs majit/majit-translate -g '*.rs' | sed -n '1,240p'Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'optimize_block relevant section:'
sed -n '721,1185p' majit/majit-translate/src/codewriter/jtransform.rs
echo 'resolve_alias and remap_value:'
sed -n '5890,5965p' majit/majit-translate/src/codewriter/jtransform.rs
echo
echo 'search upstream dependency tracker / dependency checks in rpython if present:'
rg -n "DependencyTracker|call_argument_vars|_collect_call_arg_vars|find.*call|getsubstruct|getfield.*call|unsupported.*inline|inline substruct" -S . --glob '!target/**' --glob '!*__pycache__/**' --glob '!*.png' | sed -n '1,220p'
echo
echo 'Locate repository search for dependency tracker / dependency variable collection:'
rg -n "DependencyTracker|_get_call_arg_vars|collect.*call.*arg|call_arg_vars|dependency.*variable|find.*variable" majit/majit-translate/src majit/majit-translate/tests python -S 2>/dev/null | sed -n '1,240p' || trueRepository: youknowone/pyre
Length of output: 50374
Track transitive call uses before allowing this field read.
call_argument_vars is a fixed flat list of original call arguments, so it misses same_as aliases and values carried by LinkArg into successor blocks. optimize_block remaps operands after this check, so nonzero inline-substructure reads can still be lowered as supported call arguments. Mark every FieldRead whose canonical value can reach any call argument, or move this check into optimize_block after alias/link arguments are processed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-translate/src/codewriter/jtransform.rs` around lines 271 - 273,
Update the FieldRead validation to account for transitive call-argument uses
rather than only the flat call_argument_vars list. Resolve same_as aliases and
values propagated through LinkArg into successor blocks before deciding whether
a FieldRead is a supported call argument, or move this validation into
optimize_block after operand remapping. Ensure every FieldRead whose canonical
value reaches any call argument is handled correctly.
Source: Coding guidelines
| /// One-pass WTF-8 pair walk behind a single residual boundary. As in the | ||
| /// scalar `lookup_where` residuals above, `lookup_where_wtf8` phi-merges its | ||
| /// cached MRO slice with the freshly computed opaque `Vec` borrow | ||
| /// (`<other> ∪ _ptr`); keeping the whole pair residual contains that merge | ||
| /// without reconstructing it with a second MRO walk. Marker | ||
| /// `_jit_look_inside_ = False` (rlib/jit.py:139). | ||
| #[majit_macros::dont_look_inside] | ||
| pub(crate) unsafe fn lookup_where_pair_wtf8_uncached( | ||
| w_type: PyObjectRef, | ||
| name: &Wtf8, | ||
| ) -> Option<(PyObjectRef, PyObjectRef)> { | ||
| lookup_where_wtf8(w_type, name) | ||
| } | ||
|
|
||
| #[inline] | ||
| pub(crate) unsafe fn lookup_in_type_wtf8_uncached( | ||
| w_type: PyObjectRef, | ||
| name: &Wtf8, | ||
| ) -> Option<PyObjectRef> { | ||
| lookup_where_pair_wtf8_uncached(w_type, name).map(|(_src, value)| value) | ||
| } | ||
|
|
||
| unsafe fn lookup_where_pair_wtf8( | ||
| w_type: PyObjectRef, | ||
| name: &Wtf8, | ||
| ) -> Option<(PyObjectRef, PyObjectRef)> { | ||
| match name.as_str() { | ||
| Ok(s) => lookup_where_pair(w_type, s), | ||
| Err(_) => lookup_where_pair_wtf8_uncached(w_type, name), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm lookup_where_pair_wtf8 is reached only from the
# METHOD_CACHE fill path (no JIT-tracing requirement forces the 2-walk form).
rg -n 'lookup_where_pair_wtf8\b' pyre/pyre-interpreter/src/baseobjspace.rs
rg -n 'fn lookup_where_pair\b' -A5 pyre/pyre-interpreter/src/baseobjspace.rsRepository: youknowone/pyre
Length of output: 523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant lookup definitions/calls and the cache fill path.
sed -n '8970,9085p' pyre/pyre-interpreter/src/baseobjspace.rs
printf '\n--- cache fill path ---\n'
sed -n '9224,9280p' pyre/pyre-interpreter/src/baseobjspace.rs
printf '\n--- type_dict lookup helpers ---\n'
rg -n 'lookup_in_type_where_uncached|lookup_where_class_uncached|lookup_in_type_where|lookup_where_class|type_dict_lookup.*wtf8|fn type_dict_lookup' pyre/pyre-interpreter/src/baseobjspace.rs -A8 -B3Repository: youknowone/pyre
Length of output: 50371
Route the valid UTF-8 branch through lookup_where_pair_wtf8_uncached.
lookup_where_pair_wtf8 sends valid UTF-8 names to lookup_where_pair, which calls lookup_in_type_where_uncached and lookup_where_class_uncached separately. Each call walks the MRO again. _cached_lookup_where_name calls this helper as the METHOD_CACHE fill fallback, so the normal UTF-8 cache-miss path now does two MRO walks while the uncommon lone-surrogate path does one. lookup_where_wtf8 resolves both UTF-8 and invalid UTF-8 in one pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/baseobjspace.rs` around lines 9038 - 9069, Update
lookup_where_pair_wtf8 so both valid and invalid UTF-8 names route through
lookup_where_pair_wtf8_uncached, replacing the valid-UTF-8 call to
lookup_where_pair. Preserve the existing Option pair return behavior and ensure
_cached_lookup_where_name uses the single-pass fallback for all names.
| pub(crate) fn w_memoryview_new_simple_with_owner( | ||
| w_backing: PyObjectRef, | ||
| w_obj: PyObjectRef, | ||
| ) -> PyObjectRef { | ||
| use pyre_object::bufferview::BufferView; | ||
| unsafe { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let sp = pyre_object::gc_roots::shadow_stack_len(); | ||
| pyre_object::gc_roots::pin_root(w_backing); | ||
| pyre_object::gc_roots::pin_root(w_obj); | ||
| let mv = pyre_object::memoryview::w_memoryview_alloc_header(false, true); | ||
| let r_backing = pyre_object::gc_roots::shadow_stack_get(sp); | ||
| let r_obj = pyre_object::gc_roots::shadow_stack_get(sp + 1); | ||
| pyre_object::bytearrayobject::w_bytearray_exports_incref(r_backing); | ||
| let length = pyre_object::bytearrayobject::w_bytearray_len(r_backing) as i64; | ||
| let backing = memoryview_backing_buffer(r_backing); | ||
| let view = BufferView::Simple { | ||
| backing, | ||
| w_obj: r_obj, | ||
| length, | ||
| }; | ||
| let view_ptr = pyre_object::memoryview::bufferview_alloc(view); | ||
| pyre_object::memoryview::w_memoryview_set_view(mv, view_ptr); | ||
| mv | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Mark w_memoryview_new_simple_with_owner unsafe and document its precondition.
The function calls w_bytearray_exports_incref and w_bytearray_len on w_backing without checking that w_backing is actually a bytearray. Every other constructor in this file that performs the same kind of caller-dependent operation is unsafe fn: w_memoryview_new_plain, w_memoryview_new_mmap, w_memoryview_new_formatted_nd, memoryview_gather_bytes. w_memoryview_new_plain additionally guards its bytearray-specific call with backing_is_bytearray(r_obj) before calling it; this function has no equivalent guard.
Because the function signature is safe, a future caller in the crate can pass a non-bytearray object without writing an unsafe block, and the mismatched-layout reads become undefined behavior. Add unsafe fn and a # Safety doc comment stating that w_backing must be a bytearray, matching the convention used by the sibling constructors.
Also consider renaming the w_obj parameter (e.g. to w_owner): in every other constructor in this file, w_obj names the exporter itself, but here it names the reported owner (a different object). Reusing the name for a different meaning is confusing next to w_memoryview_new_plain's w_obj.
🛡️ Proposed fix
-pub(crate) fn w_memoryview_new_simple_with_owner(
- w_backing: PyObjectRef,
- w_obj: PyObjectRef,
-) -> PyObjectRef {
+/// # Safety
+/// `w_backing` must be a `bytearray` object.
+pub(crate) unsafe fn w_memoryview_new_simple_with_owner(
+ w_backing: PyObjectRef,
+ w_owner: PyObjectRef,
+) -> PyObjectRef {
use pyre_object::bufferview::BufferView;
unsafe {
let _roots = pyre_object::gc_roots::push_roots();
let sp = pyre_object::gc_roots::shadow_stack_len();
pyre_object::gc_roots::pin_root(w_backing);
- pyre_object::gc_roots::pin_root(w_obj);
+ pyre_object::gc_roots::pin_root(w_owner);
let mv = pyre_object::memoryview::w_memoryview_alloc_header(false, true);
let r_backing = pyre_object::gc_roots::shadow_stack_get(sp);
let r_obj = pyre_object::gc_roots::shadow_stack_get(sp + 1);
+ debug_assert!(pyre_object::bytearrayobject::is_bytearray(r_backing));
pyre_object::bytearrayobject::w_bytearray_exports_incref(r_backing);And in bytesio.rs:
- Ok(crate::builtins::w_memoryview_new_simple_with_owner(
- self.buffer,
- self.self_obj(),
- ))
+ Ok(unsafe {
+ crate::builtins::w_memoryview_new_simple_with_owner(self.buffer, self.self_obj())
+ })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/builtins.rs` around lines 367 - 392, Mark
w_memoryview_new_simple_with_owner as unsafe and add a # Safety documentation
comment stating that w_backing must be a bytearray before calling
w_bytearray_exports_incref or w_bytearray_len. Update its callers to use an
unsafe block as required, and rename the w_obj parameter to w_owner if
consistent with the surrounding constructors and update its use in
BufferView::Simple.
`attr_error_wtf8` puts the name between the quotes verbatim, which the CI parity review read as a dropped `%R`. Measured on both references: `getattr(Sub, '\udcfe')` reports the lone surrogate itself on 3.14 and the six-character escape text through `descroperation.py:58`. The comment records the measurement so the rendering is not restored to the repr form. Comment-only change. Assisted-by: Claude
…essed The opcode-emission loop refreshed `jf_ptr` with `get_pinned_reg` before every operation, and `sync_ref_root_var` took an already-materialized pointer even though it stores nothing when the variable owns no ref-root slot. `get_pinned_reg` carries `other_side_effects()` and lowers to a `MovFromPReg` that `is_move()` excludes from coalescing, so neither DCE nor regalloc2 removed the unused ones. The refresh now runs for the operations that address the frame; the call-like arms that already mint their own pointer next to the call keep doing so, and `sync_ref_root_var` mints one lazily per straight-line region through a shared cache. Unused `ForceToken` results no longer materialize a pointer at all. On `call_loop_local_function` the emitted CLIF goes from 102 `get_pinned_reg` (54 with no uses) to 34 (none unused); the loop preamble block drops from 87 instructions to 72. Startup-subtracted, interleaved, 9 rounds at N=480000000 on aarch64 macOS, control and variant built back-to-back from the same `build/llbc`: the cranelift loop goes 0.3722s -> 0.3615s and cranelift/dynasm goes 1.037 -> 1.007. No jit-stats row moves: check.py reads 391/391 on dynasm, 391/391 on cranelift and 387/387 on wasm. Assisted-by: Claude
…r caller-free resolvers `BlackholeInterpreter::descrs` and `BlackholeInterpBuilder::descrs` were owned `Vec<BhDescr>`, so `acquire_interp` and `clone_context_from` deep-copied the whole table and the sole producer handed it over with `all_descrs().to_vec()`. `blackhole.py:288` binds `builder.descrs` by reference and `:102-103` stores the assembler list itself; `:154` is the only consumer and it only reads. Both fields are now `&'static [BhDescr]` and the three copy sites are plain aliases. The empty default is `&[]`, matching `:280 EMPTY_LIST_I = [] # shared`. The four resolver mutators that were the only reason the field was owned have no callers anywhere in the tree and are deleted: `BlackholeInterpreter::resolve_field_offsets` / `resolve_jitcode_fnaddrs` and `BlackholeInterpBuilder::resolve_jitcode_fnaddrs` / `resolve_field_offsets`. `pyre/pyre-jit/src/call_jit.rs:1023` named one of them in a doc line; that line is removed and `resolve_field_offset` there is now provably unreachable. `setup_jitdrivers_sd`, `install_global_build_descr_pool`, `ALL_DESCRS` and `jitdrivers_sd` are untouched, as are the three descr resolvers (`runtime_bh_descr`, `read_descr`, the nested inline-call handler), which keep three different fallback chains. Two aliasing tests replace assertions that passed vacuously: the builder test in `jitcode_runtime.rs` pins `ptr::eq` against `all_descrs()` for both the builder and an acquired interpreter, and `blackhole.rs`'s `clone_context_from` test now gives its parent a non-empty table before asserting the alias — its previous `assert_eq!(len, len)` compared 0 to 0. This removes 15 full-table copies on the multi-frame blackhole adoption path and 0 copies on every measured benchmark. It is a correctness-neutral allocation cleanup, not a perf-gate lever; no bench row is attached and no jitstats baseline is re-recorded. Assisted-by: Claude
…ering already makes `AbstractStructPtrInfo._force_elements` (info.py:217-225) emits one SETFIELD_GC per entry of `descr.get_all_fielddescrs()`, and `typeptr` is not in that list — which is why `rewrite.py:479-484` can own the type pointer alone. pyre's `all_fielddescrs()` / `gc_fielddescrs()` do include `w_class`, so `force_box_impl`'s two field loops emitted a store the allocation lowering was about to make anyway: on `bench/synth/type_immutable_reject.py` eight allocations each carried two offset-8 class-pointer stores. `w_class_store_is_covered_by_alloc` (info.rs:24-50) skips the emit only when the allocation lowering provably writes the same bytes: the trace field descr is `is_w_class()`, the size descr's `w_class_obj()` is non-zero, the FIRST `is_w_class` entry of `gc_fielddescrs()` — the same `find` the rewriter selects with — agrees on both `offset()` and `field_size()`, and the forced value is a constant `Value::Ref` equal to that class pointer. A reassigned `__class__`, a non-constant value, or a descr-offset disagreement still emits the store. One helper, called from both the VirtualStruct (info.rs:1160) and Virtual (:1211) loops. This is the same "the value already there" elision as `heap.py:88-101`, at the same layer. The eager `w_class` init in `handle_new` moves out of the `OpCode::NewWithVtable` guard (rewrite.rs:1208-1221). `clear_gc_fields` runs for both fixed-size opcodes and its `is_w_class` skip has no opcode guard, so a plain `New` with a non-zero `w_class_obj()` previously received neither the eager store nor the delayed NULL; with the elision above that slot would have been written only by the nursery's zeroing. The vtable store stays under the `NewWithVtable` guard, as `rewrite.py:482`. Also corrects two `malloc_zero_filled` doc comments that claimed production is always `true`; both backends set it to `false` whenever a real collector is installed (cranelift compiler.rs:8303, dynasm runner.rs:1704). Counts on `type_immutable_reject.py` with N=3000, both binaries built from 6073a3b0686 against the same LLBC: `[cl-gcstore]` 272 -> 264, `pre-backend` 141 -> 133 ops, output unchanged at 6000, and every one of the sixteen offset-8 bases ends with exactly one non-zero class store. wasm runs no GC-rewriter pass; its `genop_new_with_vtable` already initialises `w_class` from the size descr, so the invariant holds there too. Assisted-by: Claude
…can use it `typeobject.py:85` gives the MethodCache ONE `names` array and `text_w` (`unicodeobject.py:133-134`) hands it raw `_utf8` bytes, so upstream has a single key type and no surrogate branch. pyre split the two: `lookup_in_type_where` cached `&str` names while every lone-surrogate name went to `lookup_in_type_wtf8`, an uncached MRO walk that minted a fresh `W_UnicodeObject` per probed class. `MethodCache.names` is now `Vec<Option<Wtf8Buf>>` and `method_hash` takes `&Wtf8` with its body unchanged — `Wtf8::as_bytes` yields the identical bytes, so every existing ascii key lands in the identical slot. `lookup_in_type_where(&str)` becomes a one-line `Wtf8::new` wrapper over the new `lookup_in_type_where_wtf8`, so there is one front door rather than two bodies. `_cached_lookup_where` reads its name through `w_str_get_wtf8` instead of `w_str_get_value`, which also removes the documented panic on a lone surrogate. The residual ABI of `_pure_lookup_where_with_method_cache` is unchanged. `lookup_in_type_wtf8` becomes `lookup_where_wtf8`, returning `(w_class, w_value)` from ONE pass as `typeobject.py:491-501 _lookup_where_all_typeobjects` does — the `&str` `lookup_where_pair` walks the MRO twice only because its two halves are single-register residuals, and that shape is deliberately not copied. Misses funnel through the same `.unwrap_or((null, null))` fill, so a surrogate negative is cached exactly like an ascii one. Only the two TYPE-receiver call sites are rerouted to the cached front door. The three instance-receiver sites keep the uncached walk: `lookup_in_type_where` promotes `w_type` unconditionally, and `type(obj)` there can be megamorphic, so that needs its own three-backend jitstats pass. Measured on this tree (dynasm, N=0 vs N=2000, lldb `--auto-continue` hit counts, a two-name surrogate getattr loop): the uncached WTF-8 pair walk is 2 -> 6 (0.002/iter) and `w_str_from_wtf8` is 2301 -> 2318 (0.0085/iter); the lookups moved onto `_cached_lookup_where_name`, +6.0/iter. `_pure_lookup_where_with_method_cache` reads 0 at both N — and an ascii control loop reads 0 too, so the elidable JIT arm is not exercised on this base at all and the per-iteration global-mutex cost that arm would carry does not arise. `loops_compiled=1 loops_aborted=0` on both probes. check.py ratios (dynasm / cranelift / wasm) before the three commits on this branch and after, same host: `synth/type_dict_surrogate` 34.1 / 37.9 / 44.4 -> 7.2 / 9.2 / 6.9; `instance_surrogate_attrs` 19.1 / 21.5 / 23.6 -> 13.4 / 14.6 / 15.8; `surrogate_kwargs` 16.7 / 19.1 / 24.2 -> 14.4 / 15.4 / 18.9. The after run is on base f828557 (#1080), which changes how the pypy floor is derived but not how the ratio is computed. dynasm 389/389, cranelift 389/389, wasm 385/385. Three Python regressions land in `pyre/extra_tests/parity_tests/` — surrogate store / update / delete invalidation, `__bases__` reassignment, and a metaclass whose `mro()` returns a non-type (the permanently `version_tag == 0` case, a live path). Each runs >= 2000 iterations. check.py does not run `parity_tests`, so these do not gate. `type_set_bases` publishes the new version tag at `typedef.rs:11342` before `w_type_set_bases` / `mro_subclasses` at `:11361-11369`; that ordering is pre-existing and untouched here. Assisted-by: Claude
…ble scalars `pypy/module/pypyjit/interp_jit.py:25-30` is `['last_instr', 'pycode', 'valuestackdepth', 'locals_cells_stack_w[*]', 'debugdata', 'w_globals']` with the closing `]` on `:30`. The table cited `:30` for `lastblock` — a line that holds no field name — and `:31` for `w_globals`, which is past the end of the literal; `w_globals` is on `:29`. `lastblock` has no `_virtualizable_` entry at all, which the doc block above the table already states, so its comment now says so instead of naming a line. Assisted-by: Claude
`getbuffer` built its memoryview with `w_memoryview_new_with_flags(self.buffer, ...)`, which derives both the backing exporter and the reported `.obj` from its single argument, so `io.BytesIO(b"abc").getbuffer().obj` was the private backing bytearray. `interp_bytesio.py:149-152` keeps the two apart: `BytesIOBuffer(self)` reads the storage while `BytesIOView.__init__` passes `w_obj=w_bytesio` to `SimpleView.__init__` (`:52-62`), so `.obj` is the BytesIO. Add `w_memoryview_new_simple_with_owner`, which takes the backing and the owner separately, and call it from `getbuffer`. The export count stays on the backing: the new entry point increfs the bytearray's `_exports`, and `memoryview_release` decrefs through `w_memoryview_backing`, which reads the view's `Buffer`, not its `w_obj`. `check_exports` is unchanged. pypy3 reads `getbuffer().obj is b` as True; CPython 3.14 reports a private `_io._BytesIOBuffer` instead. Assisted-by: Claude
…icts
`type_immutable_reject.py:11-13` said the raising STORE_ATTR/DELETE_ATTR
makes the JIT "deopt into the blackhole" every iteration. The baselines
beside it read `loops_compiled=1 guard_failures=1 bridges_compiled=0` on
all three backends — one bailout for 200000 iterations.
`try_walker_trace_immutable_type_attr_raise` (specialize.rs:9717) folds
the raise into a `NewWithVtable` + `SetfieldGc` construction routed
through `SubRaise`, so the raise and its catch are paid inside the
compiled loop. The comment now says that.
`inline_call.rs` justified the `defs_w` identity `GuardValue` with a
"`GuardValue` over an `arraylen_gc` leaves the guard's only argument dead
after it" mechanism. The replacement was implemented and reverted: it
answers correctly on every defaults shape but segfaults
`synth/pickle_terminal_raise_resume` deterministically, and the same
build with the class guard kept and this `GuardValue` restored is clean,
so the length guard is what is unsound. The dead-argument mechanism is
not what the code shows, so the comment now records the measurement
instead of the mechanism.
The same comment's cost claim ("only costs the shape that builds the
callee in the caller's own loop AND omits an argument it has a default
for") overstates it. `codegen.py:582-590 _visit_defaults` takes the
`_tuple_of_consts` branch for an all-constant defaults list, and pyre's
compiler emits the same single `LOAD_CONST (None, 7)` where a
non-constant default gets `BUILD_TUPLE` — so a loop-local `def` with
literal defaults hands out one code constant and the identity guard never
fails. `make_function_inline`, the only loop-local `def` with a default
in `bench/`, records `guard_failures=1`.
`CalleeReplaySafety::DeferredCall` carried no note that it and the
nested-residual abort are one contract; the enforcer at fbw_state.rs:1413
states the promise from the other side only. The variant now records it,
that the axis is the executed-effect delta rather than raising, and that
`look_inside_graph` (`codewriter/policy.py:48`) and `can_inline_callable`
(`warmstate.py:669`) are upstream's static-decision counterparts.
Comment-only change.
Assisted-by: Claude
`PYFRAME_VABLE_FIELDS` listed six scalars; `interp_jit.py:25-30` lists
five plus one array. `lastblock` was the extra one. `rg lastblock
pypy/` returns no hits: the vendored PyPy is 3.11, which has no
`PyFrame.lastblock`, no block stack and no `FrameBlock`, and unwinds via
`pyopcode.py:152 lookup_exceptiontable`. On pyre's side no production
path writes it either — `setup_finally` / `setup_except` / `pop_block`
(`pyre-interpreter/src/eval.rs`) are the only `append_block` callers and
the 3.14 bytecode emits no SETUP_FINALLY / SETUP_EXCEPT / POP_BLOCK.
Removed:
- the `("lastblock", 4)` entry; `w_globals` renumbers 5 -> 4, and
`VABLE_NAMESPACE_FIELD_IDX` (`jit/codewriter.rs`) follows;
- `VABLE_STATIC_FIELD_DESCR_SLOTS` 6 -> 5 (`majit-ir/src/descr.rs`);
- the `lastblock: Ref` inputarg and the
`lastblock: ref @ PYFRAME_LASTBLOCK_OFFSET` field from
`virtualizable_gen.rs`;
- `vable_lastblock` and the three walk-end flush gates that compared it
against `*(frame_ptr + PYFRAME_LASTBLOCK_OFFSET)`
(`flush_walk_end_state_to_frame_inner`,
`flush_walk_end_state_at_outer_call`,
`can_flush_walk_end_state_after_outer_call`) — with no writer the
comparison could not fail;
- the unused `pyframe_lastblock_descr()`.
Kept: `PYFRAME_LASTBLOCK_OFFSET`, the heap field, its `"PyFrame.lastblock"`
entry in `PYFRAME_DESCR_GROUP` and its GC root slot.
`flatten_descr_by_ptr` probed `0u16..6` as a literal and panicked with
`idx=5 exceeds VABLE_STATIC_FIELD_DESCR_SLOTS=5` on every trace; the
bound now reads `NUM_VABLE_SCALARS`. `virtualizable/{mod,derive}.rs`
cited `interp_jit.py:25-31`, corrected to `:25-30`.
`test_setup_bridge_sym_preserves_resumed_stack_tail` drops the
`lastblock` slot from its `fail_values` / `fail_types` fixture and shifts
the `RebuiltValue::Box` indices down by one to match.
check.py: dynasm 391/391, cranelift 391/391, wasm 387/387, no jitstats
baseline moved. `cargo test --all --no-default-features --features
dynasm` green.
Assisted-by: Claude
…cape `dispatch_via_miframe`'s carrier-raise-escape arm published the exit `last_instr` and terminated with the raise but never emitted `gen_store_back_in_vable`. `record_top_level_application_traceback` performs that write only concretely, for the recording pass, so a compiled bridge left the frame's `locals_cells_stack_w` holding whatever its entry wrote, and a `tb_frame.f_locals` or `sys._getframe()` read on the way out saw every post-entry local as unbound. The walk-level twin in `jitcode_dispatch/mod.rs` already calls `fbw_force_virtualizable_before_return` in the same position. Assisted-by: Claude
`warn_if_llbc_stale` becomes `fail_if_llbc_stale` and the `PYRE_LLBC_STRICT` gate is inverted: a fingerprint mismatch now goes out as `cargo::error` and exits 1 by default, and `PYRE_LLBC_STRICT=0` selects the previous `cargo::warning`. `PYRE_LLBC_SKIP_FINGERPRINT_CHECK` still skips the comparison entirely. Assisted-by: Claude
A field whose type is an inlined by-value substructure owns no flattened FieldDescr, so `fielddescrof` fell through to the raw struct-layout row and emitted `getfield_gc_r(base, offsetof(sub))` — an 8-byte load at the address `&x.sub` should have produced. `rewrite_op_getsubstruct` (`jtransform.py:942-950`) emits `int_add(base, offsetof)` there and refuses outright when the structure is GC-managed (`:945-946`). `inline_substruct_field_offset` shares `fielddescrof`'s lookup order and reports that shape. `rewrite_op_getfield` now aliases the read to its base at offset zero, and prepends a result-less `abort/` when the offset is nonzero and the result is consumed as a call argument — the shape whose callee dereferences the value as a raw pointer. The abort carries no result because `abort/>r` and `abort/>i` only advance the pc. `import random` plus `for i in range(20000): random.random()` exited 139 on 6 of 6 runs and now exits 0; `lib-python/3/test/test_float.py` did the same. Assisted-by: Claude
… registry `w_type_new_builtin` allocates with `w_class` null, and the sweep at the end of `init_typeobjects` fills the slot only for the types held in `TYPEOBJECT_CACHE`. Four builder families construct builtin type objects that never enter that registry — `getset_descriptor_type()`, `make_exc_class`, `posix.DirEntry`, and the `py_class_typed!` / `#[pyre_class]` natives — so those type objects kept a null `w_class`. `typedef::r#type` falls back to `gettypefor(ob_type)`, so the null is not visible from Python. Route the three `w_type_new_builtin` call sites through `new_builtin_typeobject`, which stamps `w_class = w_type()`, and chain `GETSET_DESCRIPTOR_TYPE` into the sweep for the one type built before the `type` typeobject is published. A hot `Cls.__name__` loop over 37 types read 10 of them at bridges_compiled=7 / guard_failures=1480 where the registry-resident ones read 0 / 1; after the change all 37 read 0 / 1. `synth/pypy_type_surface` moves from bridges_compiled=102 guard_failures=20498 to bridges_compiled=5 guard_failures=1012 against a recorded 5 / 1011, and `pyre/check.py` reports dynasm 405/405, cranelift 405/405, wasm 401/401. Assisted-by: Claude
8301fa3 to
e4d19ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4d19ef4f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return RewriteResult::Replace(vec![ | ||
| SpaceOperation { | ||
| result: None, | ||
| kind: OpKind::Abort { | ||
| kind: crate::model::UnknownKind::UnsupportedExpr { |
There was a problem hiding this comment.
Lower inline substructures instead of inserting an abort
When a nonzero-offset inline substructure is passed directly to a call in a portal graph, this inserts OpKind::Abort; the emitted abort/ is handled as AbortMarkerReached and rejects the entire trace whenever execution reaches it, rather than merely leaving the enclosing call residual as the comment claims. Such a hot region therefore cannot compile until the substructure is represented correctly or the call is classified as residual before entering this graph; deliberately substituting a runtime abort for the upstream getsubstruct lowering is also the kind of structural shortcut the repository's parity rules prohibit.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
Summary
Seven independent changes: one interpreter defect fix, one method-cache port, two
allocation/codegen cleanups, one cranelift codegen fix, and two comment corrections.
BytesIO.getbuffer().objreported the private backing bytearraygetbufferbuilt its memoryview withw_memoryview_new_with_flags(self.buffer, ...),which derives both the backing exporter and the reported
.objfrom its singleargument.
interp_bytesio.py:149-152keeps the two apart —BytesIOBuffer(self)reads thestorage while
BytesIOView.__init__passesw_obj=w_bytesiotoSimpleView.__init__(
:52-62). A neww_memoryview_new_simple_with_ownertakes the backing and the ownerseparately. Export accounting stays on the backing: the entry point increfs the
bytearray's
_exportsandmemoryview_releasedecrefs throughw_memoryview_backing, which reads the view'sBuffer, not itsw_obj.CPython 3.14 reports a private
_io._BytesIOBufferhere; pypy3 reports the BytesIO,and that is the side ported.
Method cache keyed by WTF-8, so surrogate attribute names can use it
typeobject.py:85gives the MethodCache ONEnamesarray andtext_w(
unicodeobject.py:133-134) hands it raw_utf8bytes — one key type, no surrogatebranch. pyre split the two:
lookup_in_type_wherecached&strnames while everylone-surrogate name went to an uncached MRO walk that minted a fresh
W_UnicodeObjectper probed class.MethodCache.namesis nowVec<Option<Wtf8Buf>>;method_hashtakes&Wtf8withits body unchanged, so every existing ascii key lands in the identical slot.
lookup_in_type_wtf8becomeslookup_where_wtf8, returning(w_class, w_value)fromONE pass as
typeobject.py:491-501 _lookup_where_all_typeobjectsdoes.Only the two TYPE-receiver call sites are rerouted; the three instance-receiver sites
keep the uncached walk (
type(obj)there can be megamorphic and needs its ownthree-backend jitstats pass).
check.py ratios (dynasm / cranelift / wasm), before → after:
synth/type_dict_surrogate34.1 / 37.9 / 44.4 → 7.2 / 9.2 / 6.9;instance_surrogate_attrs19.1 / 21.5 / 23.6 → 13.4 / 14.6 / 15.8;surrogate_kwargs16.7 / 19.1 / 24.2 → 14.4 / 15.4 / 18.9.Three Python regressions land in
pyre/extra_tests/parity_tests/.optimizeopt: stop emitting the
w_classSetfieldGcallocation lowering already makesAbstractStructPtrInfo._force_elements(info.py:217-225) emits one SETFIELD_GC perentry of
descr.get_all_fielddescrs(), andtypeptris not in that list — which iswhy
rewrite.py:479-484can own the type pointer alone. pyre'sall_fielddescrs()does include
w_class, soforce_box_implemitted a store the allocation loweringwas about to make anyway.
w_class_store_is_covered_by_allocskips the emit only when the lowering provablywrites the same bytes (descr
is_w_class(), non-zerow_class_obj(), the samefindthe rewriter uses agreeing on
offset()andfield_size(), and a constantValue::Refequal to that class pointer). A reassigned__class__still emits.type_immutable_reject.pyat N=3000:[cl-gcstore]272 → 264,pre-backend141 →133 ops, output unchanged.
blackhole: share the descr table as
&'static [BhDescr]blackhole.py:288bindsbuilder.descrsby reference and:102-103stores theassembler list itself;
:154only reads. Both fields become&'static [BhDescr]andthe three copy sites are plain aliases; the empty default is
&[], matching:280 EMPTY_LIST_I = [] # shared. The four resolver mutators that were the onlyreason the field was owned have no callers in the tree and are deleted.
Two aliasing tests replace assertions that passed vacuously —
clone_context_from'sprevious
assert_eq!(len, len)compared 0 to 0.Correctness-neutral allocation cleanup; no bench row attached, no baseline re-recorded.
cranelift: emit the pinned-register read only where the frame is addressed
The opcode-emission loop refreshed
jf_ptrwithget_pinned_regbefore everyoperation.
get_pinned_regcarriesother_side_effects()and lowers to aMovFromPRegthatis_move()excludes from coalescing, so neither DCE nor regalloc2removed the unused ones.
On
call_loop_local_functionthe emitted CLIF goes from 102get_pinned_reg(54 withno uses) to 34 (none unused); the loop preamble block drops from 87 instructions to
72. Startup-subtracted, interleaved, 9 rounds at N=480000000 on aarch64 macOS: the
cranelift loop goes 0.3722s → 0.3615s and cranelift/dynasm goes 1.037 → 1.007.
Two comment corrections
virtualizable_spec.rscitedinterp_jit.py:30forlastblock(a line holding theclosing
]) and:31forw_globals(past the end of the literal; it is on:29).lastblockhas no_virtualizable_entry at all, so its comment now says so.The layout question — whether pyre needs a 6th virtualizable scalar upstream does
not list — is deliberately left open; this commit is comment-only.
attr_error_wtf8puts the name between the quotes verbatim, which the CI parityreview read as a dropped
%R. Measured on both references:getattr(Sub, '\udcfe')reports the lone surrogate itself on 3.14 and the six-character escape text through
descroperation.py:58.Verification
Full gate on the pre-rebase base (
678319fcf23):pyre/check.pydynasm 391/391,cranelift 391/391, wasm 387/387 — ALL PASSED 3/3;
cargo test --all --no-default-features --features dynasmrc=0, 101 test binaries ok, zero failures;cargo fmt --all -- --checkrc=0.Rebased onto
07e6c4aff3e(#1074) withgit range-diffreporting all seven commitsidentical; the gate is being re-run on the new base and CI covers it here.
Self-review
Assisted-bySummary by CodeRabbit
Bug Fixes
BytesIO.getbuffer()ownership and writable-buffer behavior.Performance
Tests